Add representative count enrichment #81
Conversation
| if p == 63 { | ||
| // p-value == 63 represents zero adjusted count | ||
| return 0.0 | ||
| } | ||
| return math.Pow(2, float64(p)) |
There was a problem hiding this comment.
[For reviewers] This is slightly different to what we do in apm-data (ref). In apm-data, the comment says that if p-value is invalid then it should have 1 rep count however, it's not true as a p-value==64 will result in rep count of 0.
There was a problem hiding this comment.
it's not true as a p-value==64 will result in rep count of 0.
I assume you meant p==63 as stated in https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/#p-value
There was a problem hiding this comment.
I assume you meant p==63 as stated in
Not quite. A p-value of 63 is a valid value, it represents a zero adjusted count (as per the specs you linked). OTOH, a p-value > 63 is invalid and should result in a rep count == 1 as per the comments in apm-data but the actual code in apm-data would give a rep count of 0 with p-value > 63
There was a problem hiding this comment.
Got it. Do you consider that a bug in apm-data?
There was a problem hiding this comment.
Honestly, the specs are a bit muddy around this so I don't think it would be a bug.
| if p == 63 { | ||
| // p-value == 63 represents zero adjusted count | ||
| return 0.0 | ||
| } | ||
| return math.Pow(2, float64(p)) |
There was a problem hiding this comment.
it's not true as a p-value==64 will result in rep count of 0.
I assume you meant p==63 as stated in https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/#p-value
2 participants
Related to https://github.com/elastic/opentelemetry-dev/issues/378